Conversation
…t which data interface it controls. Rebased onto the updated main feature branch.
… host consistently
|
Hi @linted I'm sorry to come back late, I did not see your PR. Thanks a lot for the feedback. I guess we have corrected bugs in parallel.... I've updated my branch since (you might have seen that) and corrected some of the bugs that you also have noticed. I just now added support for set_line_coding and set_control_line so you might have a look if that beneficial for you too? Regarding the ring buffer, I settled for just creating a bytearray each time Maybe I've overseen some of the issues you've pointed out. Would you mind to comment on the PR projectgus#2 ? I think that's better since @projectgus can also share his view. |
hoihu
left a comment
hoihu
left a comment
There was a problem hiding this comment.
There are a few bugs solved in the PR, thanks!
I can correct those obvious ones with reference to you. Otherwise, would be good to split the ringbuffer addition from the rest and place a separate PR.
| desc = ustruct.pack("<BBBBBBBB", 8, _ITF_ASSOCIATION_DESC_TYPE, itf_idx, 2, | ||
| _CDC_ITF_CONTROL_CLASS, _CDC_ITF_CONTROL_SUBCLASS, | ||
| _CDC_ITF_CONTROL_PROT, 0) # "IAD" | ||
| _CDC_ITF_CONTROL_PROT, 4) # "IAD" |
There was a problem hiding this comment.
As I understand, this is an index to a string descriptor describing the function. Do we have a string descriptor for that? Otherwise this value is 0.
There was a problem hiding this comment.
I honestly don't remember why I made it 4, just that it worked.
| desc += ustruct.pack("<BBBBB", 5, _CS_DESC_TYPE, 1, 0, 1) # "Call Management" | ||
| desc += ustruct.pack("<BBBB", 4, _CS_DESC_TYPE, 2, 2) # "Abstract Control" | ||
| desc += ustruct.pack("<BBBH", 5, _CS_DESC_TYPE, 6, itf_idx, 1) # "Union" | ||
| desc += ustruct.pack("<BBBBB", 5, _CS_DESC_TYPE, 6, itf_idx, itf_idx+1) # "Union" |
There was a problem hiding this comment.
right, I corrected the itf_idx+1 already, but missed the struct pack pattern.
There was a problem hiding this comment.
This was the big problem that fixed most things. It took me a while to realize it as well.
| if self.ep_in == None: | ||
| return | ||
| self.submit_xfer(self.ep_in, data) |
| # XXX OUT = 0x00 but is defined as 0x80? | ||
| self.ep_in = (ep_addr) | EP_IN_FLAG | ||
| self.ep_out = (ep_addr) & ~EP_IN_FLAG | ||
|
|
There was a problem hiding this comment.
I'm not sure about this one. As I recall, the CDC spec specifically asked for two endpoints for the data interface. So in my latest version it looks like
self.ep_in = (ep_addr + 1) | EP_IN_FLAG
self.ep_out = (ep_addr + 2)
| self.ep_in = (ep_addr) | EP_IN_FLAG | ||
| desc = endpoint_descriptor(self.ep_in, "interrupt", 8, 16) | ||
| return (desc, [], (self.ep_in,)) |
|
Thanks @linted for the PR and @hoihu for the heads-up. I confess that I have my own version of this branch as a WIP as well. It has some of the same fixes, plus some deeper changes to support using the CDC as a Python stream. 🤦 That's my fault for not being more proactive about pushing it, sorry. I'll go over these changes and see if there's anything that applies to the branch I have, but hopefully push that today or tomorrow so you can both take a look, too. |
|
@projectgus all good - I havent looked too much into supporting stream interfaces. Looking forward to the new stuff! and yeah ChatGPT advices have to be taken with a grain of salt 😀. But to defend „him“ I actually got some decent advice when asking USB low level staff. I might have to check wireshark then for the serial_state support |
|
I've pushed where I'm up to, here: https://github.com/projectgus/micropython-lib/tree/feature/usbd_python_cdc Although this still doesn't really work. Most of the effort I've been spending to find a memory-efficient way to buffer read and write operations to USB. I have something but I'm still not particularly happy with it. @linted I cherry-picked one of your fixes from this branch as well, thanks! |
|
Since there has been significant work on other branches, I don't think this PR needs to remain open. I'll go ahead and close it. |
3 participants
Fixed an major error in the _CS_DESC_TYPE message being sent by the CDCControlInterface. Changed to take advantage of the ring buffer found in the midi interface to implement a better read cache mechanism.